Skip to content

pull request#17

Open
sashaTribe wants to merge 4 commits intoalexnaylor99:mainfrom
sashaTribe:main
Open

pull request#17
sashaTribe wants to merge 4 commits intoalexnaylor99:mainfrom
sashaTribe:main

Conversation

@sashaTribe
Copy link

No description provided.

Holds scripts to calculate weekly averages and compare to the latest stock price as well as being able to communicate with IFTTT.
This script contains functions that fetches data from the APIs, calculates differences, and communicates with my IFTTT
communicates with IFTTT by posting a request to trigger
an email to the assigned Gmail accounts with the IFTTT
"""
api_key = "nmSjoGFt5z5ExJ4bLUMBcr0JozQykoHC2TB5dUNdUT4" # my api key for authentication
Copy link

@pgrach pgrach Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security: Your API keys are hardcoded in your code. .env + .gitignore would help to avoid committing your key to the public repo.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a really good point, thank you

json_file = "real_time_stock_data" # name of json file
recent_stock_prices = []

try:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error Handling future suggestions: if an API call fails, you might want to retry it a few times before giving up.

The reason 5 is because the stock market is only open
on weekdays.
"""
first_five_items = {k: dict[k] for k in list(dict)[:5]}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we sure there will always be at least 5 items in the dictionary? What if there are less than 5, will it raise an error?


def convert_usd_to_gbp(price_to_convert):
# a simple function to convert USD to GBP
return price_to_convert/1.23
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've hardcoded the conversion rate from USD to GBP =1.23. This can be problematic if the conversion rate changes. Consider fetching the conversion rate dynamically

print("Yes there has been a drop of prices")
else:
data = {
'Value1':'No updates on the stocks',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

email triggering: If there's no significant decrease, an email is still sent with the message "No updates on the stocks" - I would avoid spamming.

total = 0
for key, value in given_dict.items():
total += return_open_val(value)
return total/5
Copy link

@pgrach pgrach Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function finalize_dict always grabs the first 5 entries from the returned data. I wonder if this may make your average in calc_weekly_average inaccurate because you're always dividing by 5 (If the market was closed for a holiday within the last 5 weekdays)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants